shortenFullyQualifiedTypes: preserve unqualified type resolution - #3037
Open
maxandersen wants to merge 2 commits into
Open
shortenFullyQualifiedTypes: preserve unqualified type resolution#3037maxandersen wants to merge 2 commits into
maxandersen wants to merge 2 commits into
Conversation
…lug#3039) Extend the AST walker to also visit MethodCallExpr and FieldAccessExpr nodes, covering static method calls (ManagementFactory.getPlatformMXBeans), static field/enum access (TimeUnit.SECONDS), and nested-type member access (CustomTypeProperty.TypeEnum.STRING). Expression context lacks type-node certainty, so two heuristics guard against false positives: 1. Known-package check — trust if the candidate package appears in existing imports, the file's own package, or java.lang. 2. Minimum-depth fallback — otherwise require >= 2 lowercase segments before the first uppercase segment, filtering variable.Field patterns. Document the safety contract (never introduce a compile error) in both source javadoc and test class javadoc. Add 9 new tests covering expression-context shortening and ambiguous cases that must be left alone.
Contributor
Author
|
given #3033 and #3039 i've updated this pr to fix those issues that require parsing deeper to catch more unnecesary fully qualified names. Given we dont have access to full compiler context there are cases where we just can't know and here we fallback to heuristics, example:
And even in that case we try and identify if there are existing imports to guide/ensure we don't break code. Full details in the javadoc and body of description. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR combines two fixes for
shortenFullyQualifiedTypes:Fix 1: Preserve unqualified type resolution (#3033)
When a file already uses a type name unqualified (e.g.
extends RandomAccessFile), the formatter must not add an import for a different fully-qualified type with the same simple name — that would silently change what the existing reference resolves to.What changed: collect all unqualified
ClassOrInterfaceTypereferences and skip shortening any FQN whose simple name matches, unless the type is already explicitly or implicitly imported.Fix 2: Shorten FQTs in expression context (#3039)
Previously,
shortenFullyQualifiedTypesonly handled type-context references (ClassOrInterfaceTypeAST nodes — declarations, generics, casts, etc.). FQTs used in expression context were silently ignored:java.lang.management.ManagementFactory.getPlatformMXBeans(…)java.util.concurrent.TimeUnit.SECONDSpkg.models.CustomTypeProperty.TypeEnum.STRINGWhat changed: the AST walker now also visits
MethodCallExprandFieldAccessExprnodes, walking their scope chains to find package-qualified type references.Safety: never introduce a compile error
Since we run without a classpath, expression-context FQTs (which JavaParser sees as expressions, not types) need extra care to avoid false positives like shortening
variable.Fieldinto a bogus import. Two heuristics guard against this:java.lang, we trust it.config.Default.VALUEorbuilder.Type.create()patterns where the first segment is likely a local variable, not a package. In theory this skips a legitimate single-segment package (a.MyType) with no matching import, but single-segment packages are virtually non-existent in practice.When we know something is ambiguous (simple name clashes with an existing import, a declared type, or an unqualified reference), we always leave it qualified.
Tests
31 tests total (9 new for expression-context, 1 new for unqualified collision):
java.lang.System.exit()